util: use blue on non-windows systems for number/bigint #18925
Conversation
nodejs-github-bot
added
the
util
| assert.strictEqual( | ||
| util.inspect(list), | ||
| 'BufferList { length: \u001b[33m0\u001b[39m }'); |
There was a problem hiding this comment.
does anyone know why this test needs to be done with colors?
There was a problem hiding this comment.
There is a custom inspect function on the BufferList. It is special in the way that it should set the color on that property in case colors = true.
There was a problem hiding this comment.
I meant why is it testing that colors work, that's effectively testing util, not bufferlist
There was a problem hiding this comment.
No, it is testing the return value of the custom inspect function of BufferList.
|
ci failure unrelated (parallel/test-http-connect) |
|
this still needs another approval |
devsnek
added
the
author ready
|
landed in 1708af3 |
devsnek
removed
the
author ready
PR-URL: #18925 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
Should this be backported to |
|
@devsnek Fwiw, this is how this looks for me…
There was much better contrast with the previous yellow :/ Maybe we should reconsider? |
|
@addaleax i think that just comes down to your terminal styling, for instance in the default macos terminal (which i don't use <.<) its much clearer as blue. fwiw gnu highlighter uses blue for numbers too so there can be an argument made for "consistency," although i don't place much stock by that |
|
@devsnek Yeah, and it’s no problem to change the colour for me. I just wanted to mention that this can noticeably decrease visibility. |
PR-URL: nodejs#18925 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Backport-PR-URL: #19180 PR-URL: #18925 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
This reverts commit 1708af3. Numbers are much more difficult to read in blue and it would be good to have a consistent output throughout all OS. PR-URL: #19256 Refs: #18925 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: nodejs#18925 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
the only reason these stopped being blue is windows, and honestly util.inspect output looks so much more balanced with blue.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
util